Skip to content

Conversation

guha-rahul
Copy link
Contributor

@guha-rahul guha-rahul commented Aug 11, 2025

What was wrong?

Py-libp2p does not have TLS. Builds on top of #700

How was it fixed?

Summary of approach.

To-Do

  • Clean up commit history
  • Add or update documentation related to these changes
  • Add entry to the release notes

TLS architecture diagram

image

@seetadev
Copy link
Contributor

@guha-rahul : Thank you so much for making key additions to the TLS PR. Reviewed them. Please try and resolve CI/CD issues.

Did a re-run of the CI/CD pipeline.

Looking forward to seeing TLS in production stage in the coming week. Appreciate your efforts.

@guha-rahul guha-rahul marked this pull request as ready for review August 18, 2025 16:59
@guha-rahul
Copy link
Contributor Author

@seetadev and @lla-dane . i believe it has all the parts of tls. I would love to see your review. I am also in the middle of implementing more tests.

@seetadev
Copy link
Contributor

seetadev commented Aug 18, 2025

@guha-rahul : This is excellent progress—great to see TLS coming together for py-libp2p! 🎉 This is indeed a big step forward for strengthening security in the stack. Having TLS fully integrated not only ensures encrypted transport but also provides identity guarantees that will be really valuable for interoperability with other libp2p implementations.

Wish to share that I am reviewing the PR in detail and will share feedback soon. Will encourage @lla-dane to share feedback too.

Wish to share some pointers as you continue in completing this key addition to py-libp2p:

  • Test Coverage: Great to hear that you’re already in the middle of adding more tests. Be sure to include both happy-path tests (successful handshakes, encrypted message exchange, re-negotiation if applicable) and failure-path tests (invalid certificates, mismatched keys, downgrade attempts). This will make the implementation much more robust.

  • Interoperability: If possible, testing against go-libp2p’s TLS implementation could validate cross-compatibility early. Even a simple handshake exchange would be a good milestone.

  • Code Structure: Keeping the handshake, session management, and crypto primitives modular will make it easier to swap or extend in the future (e.g., QUIC + TLS).

  • Debuggability: TLS failures can be opaque, so having clear logs at the debug level for handshake progress and error conditions will help others (and you) in troubleshooting.

  • Docs/Comments: Since TLS is complex, leaving detailed inline comments (and perhaps a short README overview) will go a long way in helping future contributors understand the design decisions.

Appreciate your initiative and efforts here. TLS is foundational, and your work is laying the groundwork for more secure and production-ready networking in py-libp2p.

Looking forward to reviewing the code and tests as they land. Will share feedback soon.

@seetadev
Copy link
Contributor

@guha-rahul : Re-ran the CI/CD pipeline. 1 test case is failing. Please try and resolve it. Great progress.

This was referenced Aug 25, 2025
@seetadev
Copy link
Contributor

@guha-rahul : Thank you for your progress in TLS Pr. Lets arrive at a good conclusion to it this week.

Comment on lines +103 to +109
with tempfile.NamedTemporaryFile("w", delete=False) as cert_file:
cert_file.write(self._cert_pem)
cert_path = cert_file.name
with tempfile.NamedTemporaryFile("w", delete=False) as key_file:
key_file.write(self._key_pem)
key_path = key_file.name
ctx.load_cert_chain(certfile=cert_path, keyfile=key_path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it might be leaving these tempfiles floating around with sensitive data in them. Can we explicitly delete them after loading?

Public key from libp2p extension

Raises:
SecurityError: If verification fails
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring says it raises SecurityError, but only ValueErrors show up in the function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants